Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[draft] Introduce 'purgeUnread' config parameter #970

Closed
wants to merge 1 commit into from

Conversation

pefimo
Copy link

@pefimo pefimo commented Dec 8, 2020

Define a new configuration parameter 'purgeUnread' to control whether unread
items can be purged. Its default value is 'false'.

Extract a method counting items to be removed and introduce another one that
ignores if items are still unread.

Rearrange 'unread' and 'starred' columns in retrieval and removal queries to
make relevant condition optional with a smaller SQL rewrite.

TODO:

  • provide GUI widget to adjust purgeUnread parameter;
  • introduce and use 'maxItemAge' parameter;
  • cover with tests.

Define a new configuration parameter 'purgeUnread' to control whether unread
items can be purged.  Its default value is 'false'.

Extract a method counting items to be removed and introduce another one that
ignores if items are still unread.

Rearrange 'unread' and 'starred' columns in retrieval and removal queries to
make relevant condition optional with a smaller SQL rewrite.
@SMillerDev SMillerDev added the API Impact API/Backend code label Dec 11, 2020
@Grotax
Copy link
Member

Grotax commented Jan 7, 2021

Hey @pefimo do you want to continue to work on this?

@pefimo
Copy link
Author

pefimo commented Jan 7, 2021

@Grotax I do, but I thought you're not interested since there was no feedback. I'd like to agree on the direction before investing too much time in something that could be rejected.

Copy link
Contributor

@anoymouserver anoymouserver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, a lot of feedback happened in #953.

Regarding your code: I would strongly recommend to use the ServiceV2/MapperV2 classes for new features, as the old ones will be removed in the near future.
Also pay attention to the failed checks.

Regarding the functionality of purging unread items: I personally wouldn't use it an my managed instances, but apparently there were some requests for this.

*
* @param string $name Name of the configuration parameter.
*/
private function readConfigParam($name) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you think it's necessary to have an extra function please also replace the config value for autoPurgeCount.

@Grotax
Copy link
Member

Grotax commented Jan 7, 2021

Well can't remember that we ever rejected a PR that the author was working on.

So far I can't say much, only thin I notice is, we don't want to use manual SQL statements anymore.

That's why a lot of the classes got a V2 version upgrading it to new nextcloud and coding standards. I don't think it makes sense to implement something new in the old style.

Also unit tests are required of course.

Is this code working already, is it deployable?

@anoymouserver
Copy link
Contributor

The optional purging of unread items was added in 023c61b and will be available as of the next version.
Currently there is no config value (see #1077) and therefor it's not done by the cronjob, but it can be triggered manually using occ news:updater:after-update --purge-unread [<purge-count>].

@pefimo pefimo closed this Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Impact API/Backend code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants